-
Notifications
You must be signed in to change notification settings - Fork 757
Bug/GitHub/wconversion-1478 Check in #1623
base: main
Are you sure you want to change the base?
Conversation
Merge main into bug branch
@@ -54,6 +54,8 @@ __host__ __device__ | |||
|
|||
// Use the input iterator's value type per https://wg21.link/P0571 | |||
using ValueType = typename thrust::iterator_value<InputIterator>::type; | |||
// Use for explicit type conversion | |||
using OutputType = typename thrust::iterator_value<OutputIterator>::type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my thinking, that the output type would make sense, but when I do include this I get compile time errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output iterators are not required to have a value type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericniebler Thanks, for those that dont have it is value type set to null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been using this pattern in similar situations: https://github.com/NVIDIA/cub/blob/769e149dde423101dec8e618511e4fac8ba7bfe7/cub/util_type.cuh#L82-L94
We can't directly use that cub trait here since this file isn't in the CUDA backend, but we could add a similar one in thrust/detail/type_traits.h
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tips, I will look into implementing that. There may still be times where when it has to pick the Fallback type (which I am sensing would be ValueType to not break anything), there may still be warning.
@@ -312,6 +312,7 @@ template <class AdaptableUnaryFunction, class Iterator, class Reference = use_de | |||
// convert to their value type before calling m_f. Note that this | |||
// disallows non-constant operations through m_f. | |||
typename thrust::iterator_value<Iterator>::type const& x = *this->base(); | |||
// The issue is that x needs to be the type specified in m_f, but that is unknown | |||
return m_f(x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried
return static_cast<typename super_t::reference>(m_f(x));
To this one, but I believe it also caused some compile time errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at this again, and not sure if there is a way (without adding extra args) to inherit the type expected by the unary operator m_f, might have to be ignored from -Wconversion?
In reference to NVIDIA/cccl#779
Creating PR to get vis on the last remaining tricky type conversions.
The two pain points are
Attaching the list of remaining warnings here:
conversionWarning47.txt
There are four spots where I had a pragma ignore "-Wconversions" because those spots were called so often they would blow up the log, and tricky to solve so I moved on. (Thus they are not present in the log above). I made attempts to create general conversions for them, but was unsuccessfully at the time.
These 4 spots are:
./detail/function.h around line 97
./detail/internal_functional.h around line 332
./detail/tuple.inl (has two spots) around line 479 and 367
I am compiling a log including these errors now, will attach after the fact.
I do not believe I made any changes to the tests themselves, only the source of the type conversions. There are some tests that deliberately test a rotating array of types against each other.
I believe a trained eye could identify the solutions to the trickier ones that have eluded me.